Skip to content

Extend tests against invalid CLI option/input mode combinations to cover evmasm import mode#16518

Merged
clonker merged 2 commits intodevelopfrom
test_import_asm_mode_command_line_parser
Mar 13, 2026
Merged

Extend tests against invalid CLI option/input mode combinations to cover evmasm import mode#16518
clonker merged 2 commits intodevelopfrom
test_import_asm_mode_command_line_parser

Conversation

@clonker
Copy link
Member

@clonker clonker commented Mar 12, 2026

Also exposes the set of experimental options from cli so we don't have to maintain it in multiple places.

See #16503 (comment)

CommandLineOptions const& options() const { return m_options; }

/// yields CLI option names (without leading "--") that require --experimental.
static std::vector<std::string> const& experimentalOptionNames();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i was itching to make this a static std::array constexpr but that would've been a bigger refactor and entailed making all the global static strings in the implementation file g_str* string views or similar and update the various string concatenations in the cli to fmt::format style. didn't seem worth the effort in the end.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean you'd replace them with global static string_views? Don't they still need to be attached to some string stored somewhere? What would that look like?

TBH I have mixed feelings about those global strings. I kept them back when I refactored the CLI, but they're clunky, not used consistently for all options and I'm not sure this really saves us all that much. The biggest benefit is I guess preventing typos when you have to use an existing option name in a bunch more places, but that's not much.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean you'd replace them with global static string_views? Don't they still need to be attached to some string stored somewhere? What would that look like?

If you write static std::string_view constexpr v{"hi"} then that string literal in there already has static storage duration and so the string view can just point to that - they have constexpr support :)

So something like this works, too:

static auto constexpr opts = std::to_array<std::string_view>({
    "A", "B", "C"
});

TBH I have mixed feelings about those global strings.

Not the biggest fan of them, either. I agree, they have dubious value here... I'd be much happier with a struct that defines the properties of an option somewhere and having a couple of these.

@clonker clonker requested review from cameel and nikola-matic March 12, 2026 13:51
@clonker clonker force-pushed the test_import_asm_mode_command_line_parser branch from 61013c6 to c2e935c Compare March 12, 2026 13:55
@clonker clonker marked this pull request as ready for review March 12, 2026 14:04
@cameel cameel changed the title Test import asm mode command line parser Extend tests against invalid CLI option/input mode combinations to cover evmasm import mode Mar 12, 2026
cameel
cameel previously approved these changes Mar 12, 2026
@clonker clonker force-pushed the test_import_asm_mode_command_line_parser branch 2 times, most recently from 1c491c2 to 73cfbfe Compare March 13, 2026 10:16
@clonker clonker force-pushed the test_import_asm_mode_command_line_parser branch from 73cfbfe to bb64ae4 Compare March 13, 2026 20:35
@clonker clonker merged commit 7e38b81 into develop Mar 13, 2026
83 checks passed
@clonker clonker deleted the test_import_asm_mode_command_line_parser branch March 13, 2026 23:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants